Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various code readability ideas #2059

Closed
wants to merge 24 commits into from

Conversation

gigamonkey
Copy link

Hey Jeremy, here are some ideas left over from my code reading refactoring plus some new ones. I'd be happy to talk about these and tweak them as needed if you're interested in incorporating them.

Note that these changes, while aimed primarily at readability, do introduce some small incompatibilities/API changes. The main one is that I got rid of the 'unset' option in preference for treating attributes passed with a undefined value as unsetting those attributes. I think this is actually righteous since json can't represent an undefined value so a model with 'foo' present but undefined will serialize the same as a model with 'foo' simply not present. I had to comment out one test case which specifically tested that you could set an attribute to undefined.

Also I axed the changed property of Model. I assume that you've kept it around for backwards compatibility so you may still be unwilling to see it go. But it seems like a wart on the public API since you're not supposed to manipulate it directly.

Anyway, had fun playing around with this. Let me know what you think.

-Peter



// The ChangeTracker is used by the Model to keep track of the state of
// changes made to the model's attributes so at to trigger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo... "so as to"

@jashkenas
Copy link
Owner

Thanks for writing this up as a pull request -- righteous and nicely OOP-y. I'll try to grab a chance to review it soon.

@krawaller
Copy link
Contributor

@gigamonkey I don't have anything constructive to add, just wanted to say - really cool refactoring! Nicely done! :)

@gigamonkey
Copy link
Author

Jeremy, any chance this is actually going to get folded in?

@philfreo
Copy link
Contributor

This refactor is pretty cool - @jashkenas thoughts?

@jashkenas
Copy link
Owner

Whoops -- sorry, I had "[backbone]" tickets filtering out of the inbox for a couple of months there. It'll be time to come back around crank through some of these soon.

@braddunbar
Copy link
Collaborator

I'm afraid this may not apply after the changes to Model#set. :(

@gigamonkey
Copy link
Author

Well, it's good to merge now and it's only been sitting around for 2 months. :-(

@tbranyen
Copy link
Collaborator

@gigamonkey to be fair, it's typically better to bring up an issue first and then submit a pull request so theres no wasted efforts. jQuery encourages this so that people don't feel bad if ideas are turned down after they put significant time into a pull request.

@gigamonkey
Copy link
Author

Sure. I mentioned it to Jeremy before I started. And I'm cool with whatever you guys want to do. But if some other change was already in progress when I submitted my PR it would have been cool for someone to say, "Hey check this other upcoming change out and see if you can work with it." And if the other change came after mine, it'd have been nice if either mine had already been merged or if the other author had been told about my change.

@tgriesser
Copy link
Collaborator

Sorry about that @gigamonkey - I think your request came in just after the decision was made to completely rewrite set and strip out queued changes entirely, and the pull request was more directed at/responded to by Jeremy, so I figured it'd be in his court to take a look through.

@braddunbar
Copy link
Collaborator

I think @tgriesser's assessment is correct, but I'm sorry you spent time on this without being informed. We're attempting to address some of these organizational failings by using milestones to track features/fixes that are currently being worked on.

@jashkenas
Copy link
Owner

I have to apologize for dropping the ball on this -- the fault is entirely mine for "commissioning" it at a poor time.

That said, I think there are a couple of issues with implementing this particular direction:

  • Ideally, we wouldn't introduce a new class that exists just to namespace some of the internal Model implementation. As the ChangeTracker is only ever instantiated within the Model constructor, it feels like it should simply be methods directly on the model itself.
  • That unset change is righteous -- you have a great point about the JSON, and we should definitely do that.
  • The model.changed property should stick around. It's useful, and is an intentionally-exposed part of the internals much like model.attributes is, or even model.id. Read-only reference properties.

So, closing, but thanks much!

@jashkenas jashkenas closed this Mar 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants